/retest

I may be wrong, but it seems to me that by deleting those lines from the code you removed the following functionality from porch:
The internal metadata store CR (kind: PackageRev) doesn't have an ownerReference to its parent PackageRevision object any more. That ownerReference exists mainly to let the garbage collector know that if the PackageRevision is deleted it can also delete the PackageRev.

Could you elaborate on the reasons and implications of removing this functionality?

I may be wrong, but it seems to me that by deleting those lines from the code you removed the following functionality from porch: The internal metadata store CR (kind: PackageRev) doesn't have an ownerReference to its parent PackageRevision object any more. That ownerReference exists mainly to let the garbage collector know that if the PackageRevision is deleted it can also delete the PackageRev.

Could you elaborate on the reasons and implications of removing this functionality?

Hey @kispaljr, thanks for your feedback.
The issue is that after an approve, when the 'main' twin gets created, intermittently, k8s adds the deletionTimestamp to it.
The suspect seemed to be the ownerRefs. Sometimes k8s sees the ownerRefs before the pkgrev is created and it looks at the pkgrev as an orphan. So upon a propose-delete, porch checks if the pkgRev has a deletionTimestamp and if it does, it removes the finalizer and calls for a delete of the pkgrev.
Initially, I didn't remove the ownerRefs, I created the pkgrev without the ownerRefs and then called for an update(adding the ownerRefs) but that didn't help as well.

Thank you for the clarification. I think I understand it much better now.
Can we find a solution that keeps the ownerReference in the PackageRev pointing to the PackageRevision, but avoid the race condition at the same time?

In the latest commit, I've reverted the removal of the ownerRefs and changed the flow in repo.go to send a notification of the packagerevision change first before creation of the packageRev.

/approve

@kispaljr does the later changes address your concerns?

sorry, I haven't got the time to review the new version till now.
The current implementation is definitely an improvement, so I support merging it.

I still cannot see however, how this change guarantees that the parent PackageRevision exists in the porch server by the time the PackageRev is created in the etcd. Please note, that I am not saying that it doesn't guarantee it, I just don't see why, and I am afraid that it only decreases the probability of the race condition to happen by delaying the creation of PackageRevs with the time it takes to send out the notifications.
I guess all I am trying to say is that an explanation of how this solves the problem should be put in a comment in the code, so that developers would understand it easier.

Since as I stated there is no doubt that this is an improvement, and the solution was tested so we are sure that in practice it solves the original issue, please ignore my theoretical thoughts above and let's merge this.

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Catalin-Stratulat-Ericsson, efiacor, kispaljr, kushnaidu, liamfallon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Merge

Hey Istvan,
I tested this change with around 150-200 package revisions and couldn't reproduce the issue even once. When the cached repo is updated, it gives enough time for the pkgRev to use a valid uid I think.

Yeah, the race condition doesn't fully disappear. If the kube-controller-manager is processing through notifications from porch much slower than processing through notifiactions from kube-apiserver's pkgrev watch, the same issue can happen.

But this can happen between any 2 resources, regardless of which component serves it. I'll check whether there is a safeguard against this in the garbage collector.